Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements GOCACHEPROG protocol support for hardcache, enabling it to serve as an external build cache program for the Go toolchain. This allows the Go compiler to delegate cache operations to hardcache instead of using the default filesystem-based cache.
Key changes:
- Added a new
internal/progpackage that implements the GOCACHEPROG protocol - Added a hidden
local usecommand to main.go that starts the cache program in GOCACHEPROG mode - Updated the Makefile to test the new functionality by building Go with hardcache as the cache program
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Adds the local use command that initializes and runs the GOCACHEPROG server with stdin/stdout |
| internal/prog/prog.go | Implements the GOCACHEPROG protocol, handling get, put, and close commands with JSON message encoding/decoding |
| internal/prog/prog_test.go | Adds basic test infrastructure for the prog package, though with limited coverage and bugs |
| internal/prog/prog_client_test.go | Provides a test client for simulating GOCACHEPROG protocol interactions |
| Makefile | Adds a test build using the new GOCACHEPROG functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pr, cw := io.Pipe() | ||
| cr, pw := io.Pipe() | ||
|
|
||
| p = New(cache, l, pr, pw) |
There was a problem hiding this comment.
Undefined variable l used. This should likely be logger(t) to match the logger helper function defined in this file.
| p = New(cache, l, pr, pw) | |
| p = New(cache, logger(t), pr, pw) |
| <-done | ||
| cancel() |
There was a problem hiding this comment.
The cleanup order is incorrect. The context is being cancelled after waiting for the goroutine to complete, but the goroutine likely needs the context cancellation to exit cleanly. The cancel() call should come before <-done to properly signal the goroutine to stop.
| <-done | |
| cancel() | |
| cancel() | |
| <-done |
| } | ||
|
|
||
| if l := len(b); int(req.BodySize) != l { | ||
| return nil, fmt.Errorf("put: expected body size %d, got %d", req.BodySize, l) |
There was a problem hiding this comment.
Inconsistent error message format. This error message is missing the "hardcache:" prefix that is used in other error messages throughout this file. Should be: "hardcache: put: expected body size %d, got %d"
| return nil, fmt.Errorf("put: expected body size %d, got %d", req.BodySize, l) | |
| return nil, fmt.Errorf("hardcache: put: expected body size %d, got %d", req.BodySize, l) |
| // Run runs the Prog until ctx is canceled, close message is received and handled, | ||
| // or an error occurs. |
There was a problem hiding this comment.
The documentation states "Run runs the Prog until ctx is canceled" but the context is never checked for cancellation in the main loop. The function only exits on EOF, close command, or errors. Consider either updating the documentation to reflect the actual behavior, or adding context cancellation handling in the main loop (e.g., using a select statement or checking ctx.Err()).
# Conflicts: # main.go
1 participant
Refs #11.